Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(seg): Improve Labelmap Statistics, Interpolation, and Threshold Configuration #1820

Closed
wants to merge 35 commits into from

Conversation

sedghi
Copy link
Member

@sedghi sedghi commented Feb 10, 2025

This PR will:

Labelmap Statistics

  • makes it work on stack viewports too
  • moves the calculation to web worker to not block ui
  • moves it to a utility function instead of a brush strategy (we should be able to calculate stats on anything not just brush instances)

Sphere Brush

  • was broken on stack viewports now it is fixed

Interpolation

  • makes it work on stack viewports too
  • moves the calculation to web worker to not block ui
  • moves it to a utility function instead of a brush strategy (we should be able to calculate stats on anything not just brush instances)

THRESHOLD -> Strategy specific threshold

Previously, Brush tools using thresholding might have used a generic THRESHOLD property within their strategySpecificConfiguration. This PR introduces a shift towards more explicit and strategy-specific threshold configurations. Instead of a single THRESHOLD property (which is NOT a strategy), you'll now configure thresholds under properties named after the specific strategy itself (e.g., THRESHOLD_INSIDE_CIRCLE, THRESHOLD_INSIDE_SPHERE, THRESHOLD_INSIDE_SPHERE_WITH_ISLAND_REMOVAL).

It is now the app-level concern to make sure threshold is synced between different tool strategies if desired

{
    configuration: {
        activeStrategy: 'THRESHOLD_INSIDE_CIRCLE', 
        strategySpecificConfiguration: {
            THRESHOLD: { // Old generic THRESHOLD property
                threshold: [50, 200], // Example threshold range
            },
        },
    },
});
  • If your activeStrategy is 'THRESHOLD_INSIDE_CIRCLE', rename THRESHOLD to THRESHOLD_INSIDE_CIRCLE.
  • If your activeStrategy is 'THRESHOLD_INSIDE_SPHERE', rename THRESHOLD to THRESHOLD_INSIDE_SPHERE.
  • If your activeStrategy is 'THRESHOLD_INSIDE_SPHERE_WITH_ISLAND_REMOVAL', rename THRESHOLD to THRESHOLD_INSIDE_SPHERE_WITH_ISLAND_REMOVAL.
After (Example - Migrated Configuration):

{
     configuration: {
         activeStrategy: 'THRESHOLD_INSIDE_CIRCLE',
        strategySpecificConfiguration: {
            useCenterSegmentIndex: true,
            THRESHOLD_INSIDE_CIRCLE: { // Renamed to strategy-specific property
                threshold: [50, 200], // Threshold settings remain the same inside
            },
        },
    },
});

sedghi added 13 commits February 6, 2025 16:57
…or brush tools

This commit introduces several improvements to the segmentation strategy configuration:
- Updated strategy-specific configuration to use active strategy as key
- Refactored dynamic threshold and threshold compositions to work with new configuration structure
- Added support for stack and volume viewport strategy data handling
- Simplified strategy data retrieval and configuration management
- Removed deprecated configuration patterns
…provements

- Introduced new `interpolateLabelmap` utility for advanced segmentation interpolation
- Created `getOrCreateSegmentationVolume` helper function
- Updated ITK image conversion utility to support custom scalar data
- Refactored segmentation interpolation strategies
- Simplified segmentation volume creation and management
- Introduced `calculateSpacingBetweenImageIds` utility for precise volume spacing calculation
- Refactored `sortImageIdsAndGetSpacing` to use new spacing calculation method
- Updated `generateVolumePropsFromImageIds` to generate default volume ID
- Exported new spacing calculation utility in index
- Introduced `generateVolumeId` method to create deterministic volume IDs from image IDs
- Added `getImageIdsForVolumeId` to retrieve image IDs associated with a volume
- Implemented `_fnv1aHash` helper method for consistent ID generation
- Created a new cache map `_imageIdsToVolumeIdCache` for tracking image-volume relationships
…generation

- Updated `getOrCreateSegmentationVolume` to handle volume ID retrieval more robustly
- Added support for generating volume IDs from labelmap image IDs
- Simplified volume lookup and creation logic
- Improved type safety with explicit type casting for representation data
Copy link

netlify bot commented Feb 10, 2025

Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit 2db10c4
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/67b8bc8e459c590008b480c8
😎 Deploy Preview https://deploy-preview-1820--cornerstone-3d-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sedghi sedghi changed the title feat/new seg stuff feat(seg): Improve Labelmap Statistics, Interpolation, and Threshold Configuration Feb 10, 2025
@sedghi
Copy link
Member Author

sedghi commented Feb 10, 2025

@wayfarer3130, could you check out peerImport versus import? I'm having trouble getting the labelmapInterpolation example to work without await import. For some reason, running peerImport inside a web worker just isn't working.

@sedghi sedghi requested a review from wayfarer3130 February 10, 2025 16:01
- Added `peerImport` utility function to handle dynamic imports of optional modules
- Updated `getItkImage`, `computeWorker`, and `polySegConverters` to use new peer import mechanism
- Centralized module import strategy to improve modularity and flexibility
* @param imageIds - Array of image IDs
* @returns A deterministic volume ID
*/
public generateVolumeId(imageIds: string[]): string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the idea, however I wonder if the hash length is sufficiently unique that we won't ever generate two hashes containing the same hash code? Would sha1 to 8 digits be better? That is natively encoded and is reasonably fast.
async function sha1(str) {
const enc = new TextEncoder();
const hash = await crypto.subtle.digest('SHA-1', enc.encode(str));
return Array.from(new Uint8Array(hash))
.map(v => v.toString(16).padStart(2, '0'))
.join('');
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, does it need to be order invariant? Should the entries be sorted first?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like crypto apis rely on secure context, so i guess we can't

wayfarer3130 and others added 9 commits February 13, 2025 10:52
- Sort image URIs in volume ID generation for consistent hashing
- Enhance scalar data update in VoxelManager to prevent side effects
- Update labelmap interpolation to use active segment index
- Deprecate labelmap interpolation strategy with warning
@wayfarer3130
Copy link
Collaborator

I'm seeing:
Warning: '@itk-wasm/morphological-contour-interpolation' module not found. Please install it separately.
when I try to run the example. Using the separate name strings doesn't seem to actually work. Is it ok if we just give it a chunk name for now and live with the fact that the binding is hard now? I don't really see a way to fix the workers other than importing them in some hand written wrapper library.

This commit introduces a new configuration mechanism to conditionally enable PolySeg and Labelmap Interpolation features:

- Created a new `config.ts` module to manage global configuration
- Updated initialization to set configuration options
- Modified worker and import logic to respect configuration flags
- Removed hardcoded example runner checks
- Simplified dynamic import strategies for optional modules
import vtkDataArray from '@kitware/vtk.js/Common/Core/DataArray';

/**
* Dynamically imports ITK WASM modules needed for labelmap interpolation
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment that this doesn't currently allow the dynamic import?

@sedghi
Copy link
Member Author

sedghi commented Feb 21, 2025

closing in favor of #1844

@sedghi sedghi closed this Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants